Add reader and writer for Puffin, indexes and stats file format by findepi · Pull Request #4537 · apache/iceberg

findepi · 2022-04-11T17:48:32Z

Format documentation: #4944

core/src/main/java/org/apache/iceberg/stats/StatsFormat.java

core/src/main/java/org/apache/iceberg/stats/BlobMetadata.java

build.gradle

core/src/main/java/org/apache/iceberg/stats/BlobMetadata.java

core/src/main/java/org/apache/iceberg/stats/StatsFormat.java

findepi · 2022-04-13T12:45:28Z

@rdblue i applied most of the comments (no more jackson-datatype-jdk8, json-related annotations, get in accessor names). I also switched over from base16-encoded test "fixtures" to test resources.

I switched writing to use ByteBuffer, this removes removed data copying upon compression.
This is as a fixup for now. Please let me know if you want me to use ByteBuffer in the reader/writer APIs as well?
It seems byte[] works there well (eg because uncompressed size is known, so no need to re-allocation), so please confirm before I apply further changes.

rdblue · 2022-04-13T16:52:35Z

@findepi, I would prefer to use ByteBuffer everywhere. If the backing buffer was easy to allocate and work with, that's great. But you're not forced to reallocate if it is not.

core/src/main/java/org/apache/iceberg/stats/FileMetadataParser.java

core/src/main/java/org/apache/iceberg/stats/StatsFormat.java

core/src/main/java/org/apache/iceberg/stats/StatsWriter.java

findepi · 2022-04-20T09:25:23Z

( squashed and rebased on top of current version of #4534, no other changes in this push )

findepi · 2022-04-20T09:54:53Z

AC
@rdblue @singhpk234 thanks for your review, let me know what else I can change.

core/src/main/java/org/apache/iceberg/stats/StatsCompressionCodec.java

core/src/main/java/org/apache/iceberg/stats/StatsFormat.java

findepi · 2022-04-22T07:41:18Z

AC & rebased after #4534 merged.

core/src/main/java/org/apache/iceberg/stats/BlobMetadata.java

findepi · 2022-06-14T08:31:46Z

Rebased after #5019 merged.

nastra

overall LGTM, just a few suggestions here and there

nastra · 2022-06-15T07:15:18Z

core/src/main/java/org/apache/iceberg/puffin/BlobMetadata.java

+    return type;
+  }
+
+  public List<Integer> inputFields() {


the spec mentions that input fields are JSON longs, so I'm just wondering whether this should be a List<Long>?

Iceberg field IDs are integers, so the implementation is chosen to be limited to integers.

We can maybe change fields | list of JSON long in the puffin spec to be list of integers.

Yes, we should update the spec to match the type used by the table spec for field IDs.

core/src/main/java/org/apache/iceberg/puffin/PuffinFormat.java

core/src/test/java/org/apache/iceberg/puffin/TestFileMetadataParser.java

core/src/test/java/org/apache/iceberg/puffin/TestPuffinFormat.java

core/src/main/java/org/apache/iceberg/puffin/PuffinReader.java

core/src/test/java/org/apache/iceberg/puffin/TestPuffinWriter.java

findepi · 2022-06-15T11:52:15Z

Thanks @rdblue and @nastra for your reviews!

Updated the code accordingly. Please take another look.

rdblue · 2022-06-15T17:56:04Z

core/src/main/java/org/apache/iceberg/puffin/PuffinWriter.java

+    ByteBuffer footerPayload = PuffinFormat.compress(footerCompression, footerJson);
+    outputStream.write(MAGIC);
+    int footerPayloadLength = footerPayload.remaining();
+    writeFully(footerPayload);


Minor: why not call IOUtil.writeFully(outputStream, footerPayload) directly rather than having a private writeFully method that just adds the output stream?

rdblue · 2022-06-15T17:58:25Z

core/src/main/java/org/apache/iceberg/io/IOUtil.java

+    if (!buffer.hasRemaining()) {
+      return;
+    }
+    byte[] chunk = new byte[WRITE_CHUNK_SIZE];


Rather than allocating every time this is called, can you create a ThreadLocal to share this buffer? Alternatively, you could pass the temporary buffer in.

Alternatively, you could pass the temporary buffer in.

This poses sizing challenge. I.e. the caller needs to provide a reasonably sized buffer.

Rather than allocating every time this is called, can you create a ThreadLocal to share this buffer?

Sure, this is feasible. Do you happen to know what would be the expected reuse ratio for such a buffer?

Alternatively we can have a static buffer pool that lends buffers to the current thread.
(assuming we have a problem that we want to fix here)

rdblue · 2022-06-15T18:02:02Z

Thanks, @findepi! This looks good to me. We can still follow up, but I think the majority of the changes are ready so I've merged it to avoid keeping a big PR outstanding.

…#4537)

github-actions bot added build core labels Apr 11, 2022

findepi mentioned this pull request Apr 11, 2022

Use memory-backed streams in tests #4534

Merged

findepi force-pushed the findepi/stats-file branch from 984dce9 to e823511 Compare April 11, 2022 18:02

findepi mentioned this pull request Apr 11, 2022

Add stats format specification apache/iceberg-docs#69

Closed

findepi commented Apr 11, 2022

View reviewed changes

core/src/main/java/org/apache/iceberg/stats/StatsFormat.java Outdated Show resolved Hide resolved